[4.21] Remove DataSource storage class fallback from DV template helper#5228
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-6
requested review from
acinko-rh,
ema-aka-young,
josemacassan,
kgoldbla,
rnetser and
vsibirsk
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-6
requested review from
dalia-frank,
dshchedr and
kshvaika
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
/verified |
|
/retest verify-bugs-are-open |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR diff summary ==="
git diff HEAD~1 --stat
echo ""
echo "=== Modified files ==="
git diff HEAD~1 --name-only
echo ""
echo "=== Diff of changed files ==="
git diff HEAD~1 --unified=5Length of output: 4286 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Modified function/method symbols ==="
git diff HEAD~1 --unified=0 | grep '^[+-]def '
echo ""
echo "=== Find data_volume_template_with_source_ref_dict callers in tests/ ==="
rg -rn 'data_volume_template_with_source_ref_dict' tests/ --include="*.py" -l
echo ""
echo "=== Find callers in conftest files ==="
rg -rn 'data_volume_template_with_source_ref_dict' --include="conftest.py" -l
echo ""
echo "=== Smoke marked files ==="
rg -l '`@pytest.mark.smoke`' tests/
echo ""
echo "=== Gating marked files ==="
rg -l '`@pytest.mark.gating`' tests/Length of output: 1529 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== All callers of data_volume_template_with_source_ref_dict ==="
rg -rn 'data_volume_template_with_source_ref_dict' --include="*.py" -C2
echo ""
echo "=== Same-file callers (find the modified file first) ==="
fd 'data_volume' --extension pyLength of output: 1029 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== All callers of data_volume_template_with_source_ref_dict ==="
rg -rn 'data_volume_template_with_source_ref_dict' -g '*.py' -C2
echo ""
echo "=== Smoke marked files ==="
rg -l '`@pytest.mark.smoke`' tests/
echo ""
echo "=== Gating marked files ==="
rg -l '`@pytest.mark.gating`' tests/
echo ""
echo "=== Check smoke/gating in golden_images test file ==="
rg -n 'smoke|gating|post_upgrade' tests/infrastructure/golden_images/test_common_templates_data_volumes.pyLength of output: 25006
Test Execution Plan Run smoke tests: True Run gating tests: True Affected tests to run
Real tests (cluster required) Error path (the fix — snapshot storage class set as default): Expected: VM created successfully using snapshot SC; no storage class mismatch error. Happy path (regression — normal storage class): Expected: VM created with the explicitly provided storage class; cluster default used when storage_class=None. |
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR diff stat ==="
git diff HEAD~1 --stat
echo ""
echo "=== Modified files ==="
git diff HEAD~1 --name-only
echo ""
echo "=== First changed file and first changed line ==="
git diff HEAD~1 --unified=0 | head -60
echo ""
echo "=== Modified symbols in utilities/ ==="
git diff HEAD~1 --unified=0 | grep '^[+-]def 'Length of output: 3099 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== All callers of data_volume_template_with_source_ref_dict ==="
rg -rn 'data_volume_template_with_source_ref_dict' -g '*.py' --include="*.py" -l
echo ""
echo "=== Smoke marked files ==="
rg -rl '`@pytest.mark.smoke`' tests/ --include="*.py"
echo ""
echo "=== Gating marked files ==="
rg -rl '`@pytest.mark.gating`' tests/ --include="*.py"Length of output: 1125 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Detailed callers with context ==="
rg -rn 'data_volume_template_with_source_ref_dict' -g '*.py' -C3
echo ""
echo "=== Check if smoke files intersect with caller files ==="
CALLER_FILES=$(rg -rl 'data_volume_template_with_source_ref_dict' -g '*.py')
SMOKE_FILES=$(rg -rl '`@pytest.mark.smoke`' tests/ --include="*.py")
echo "Caller files:"
echo "$CALLER_FILES"
echo ""
echo "Smoke files:"
echo "$SMOKE_FILES"
echo ""
echo "Intersection:"
comm -12 <(echo "$CALLER_FILES" | sort) <(echo "$SMOKE_FILES" | sort)Length of output: 29775 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR details to find the first file and first changed line number for inline comment
gh pr view 5228 --repo RedHatQE/openshift-virtualization-tests --json files
echo ""
echo "=== Get diff to find exact line numbers ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5228/files --jq '.[0] | {filename: .filename, patch: .patch}'Length of output: 1330 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the latest commit SHA for the PR
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5228 --jq '{head_sha: .head.sha, base_sha: .base.sha}'
echo ""
# List files changed
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5228/files --jq '.[] | {filename: .filename, changes: .changes, patch: .patch[0:300]}'Length of output: 2045 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Smoke marked files ==="
rg -rl '`@pytest.mark.smoke`' tests/
echo ""
echo "=== Gating marked files ==="
rg -rl '`@pytest.mark.gating`' tests/
echo ""
echo "=== Check instance_types/supported_os/utils.py callers for smoke ==="
rg -rl 'from tests.infrastructure.instance_types.supported_os.utils import\|from tests\.infrastructure\.instance_types\.supported_os import utils' tests/ || true
rg -rn 'supported_os.utils\|from.*supported_os.*utils' tests/ -C2 || true
echo ""
echo "=== Check virt/utils.py callers ==="
rg -rn 'from tests.virt.utils import\|from tests\.virt import.*utils' tests/ -l || trueLength of output: 6335 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Post inline review comment on first changed line of tests/conftest.py
# Position 5 = first added line (+@pytest.fixture()) after hunk header + 3 context lines
PLAN="**Test Execution Plan**
**Run smoke tests: True**
Dependency path: \`tests/infrastructure/instance_types/supported_os/test_rhel_os.py\` (smoke) → \`tests/infrastructure/instance_types/supported_os/utils.py\` → \`n(data_source=DataSource(...))\` with no \`storage_class\` → behavior changed (no longer falls back to DataSource's storageClassName).
**Run gating tests: True**
Dependency path: \`tests/virt/upgrade/test_upgrade_virt.py\` (gating) → \`tests/virt/upgrade/conftest.py\` → \`n(data_source=datasources_for_upgrade[0])\` with no \`storage_class\` → behavior changed.
**Affected tests to run**
| Path | Notes |
|---|---|
| \`tests/infrastructure/golden_images/test_common_templates_data_volumes.py\` | Direct change — now passes explicit \`storage_class_name_scope_function\` |
| \`tests/infrastructure/instance_types/supported_os/\` | (smoke) utils.py calls \`n()\` without storage_class |
| \`tests/virt/upgrade/test_upgrade_virt.py\` | (gating) conftest calls \`n()\` without storage_class |
| \`tests/infrastructure/golden_images/update_boot_source/\` | conftest + test_boot_sources_vm.py + test_ssp_data_import_crons.py call \`n()\` without storage_class |
| \`tests/storage/data_import_cron/\` | conftest calls \`n()\` without storage_class |
| \`tests/observability/metrics/\` | conftest calls \`n()\` without storage_class (two fixtures) |
| \`tests/infrastructure/numa/\` | conftest calls \`n()\` without storage_class |
**Real tests (cluster required)**
Error path (the fix — snapshot storage class as default):
\`\`\`
pytest tests/infrastructure/golden_images/test_common_templates_data_volumes.py::test_vm_from_golden_image_cluster_default_storage_class --storage-class-matrix=\$SNAPSHOT_SC
\`\`\`
Expected: VM created successfully; no storage class mismatch with snapshot SC.
Happy path (regression — normal storage class):
\`\`\`
pytest tests/infrastructure/golden_images/test_common_templates_data_volumes.py::test_vm_dv_with_different_sc --storage-class-matrix=\$STANDARD_SC
\`\`\`
Expected: VM created with explicitly provided storage class; cluster default used when \`storage_class=None\`."
gh api \
repos/RedHatQE/openshift-virtualization-tests/pulls/5228/reviews \
--method POST \
--field commit_id="317f0cbcec1cb5c3873c9f8b22eb2da4ba3478dc" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/conftest.py" \
--field "comments[][position]=5" \
--field "comments[][body]=$PLAN"
echo "Review posted"Length of output: 4475 The Test Execution Plan has been posted as an inline review comment on the first changed line of |
|
/cherry-pick cnv-4.20 |
1 similar comment
|
/cherry-pick cnv-4.20 |
|
Cherry-picked PR [4.21] Remove DataSource storage class fallback from DV template helper into cnv-4.20: #5313 |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:cnv-4.21 published |
|
/cherry-pick cnv-4.19 |
|
Cherry-picked PR [4.21] Remove DataSource storage class fallback from DV template helper into cnv-4.19: #5358 |
|
/cherry-pick cnv-4.18 |
|
Manual cherry-pick is needed |
9 participants
What this PR does / why we need it:
When calling
data_volume_template_with_source_ref_dictwe should either use it with explicitstorage_classfield, or omit the field and use the default storage class.Relying on the storage class of the DataSources can cause miss interpretations, while also causing issues since it is not possible to match the storage class to the DataSource, if a snap shot storage class is used.
Which issue(s) this PR fixes:
Failing tests when snapshot storage classes are set.
Special notes for reviewer:
Cherry-pick from main branch, original PR:
#4985
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88988